[RISCV] Relax destination instruction dag operand matching in CompresInstEmitter #148660
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-tablegen @llvm/pr-subscribers-backend-risc-v Author: Sudharsan Veeravalli (svs-quic) ChangesWe have some 48-bit instructions in the For eg. the I think we can remove the check that only source instruction operands can mismatch with the corresponding DAG operands and rely on the fact that we check if the DAG register operand type is a subclass of the instruction register operand type. Full diff: https://github.com/llvm/llvm-project/pull/148660.diff 3 Files Affected:
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoXqci.td b/llvm/lib/Target/RISCV/RISCVInstrInfoXqci.td
index 0f222988114b8..4cbb3085207f9 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfoXqci.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoXqci.td
@@ -1638,6 +1638,24 @@ def : CompressPat<(QC_E_ADDAI X2, simm10_lsb0000nonzero:$imm),
(C_ADDI16SP X2, simm10_lsb0000nonzero:$imm)>;
def : CompressPat<(QC_E_ADDI X2, X2, simm10_lsb0000nonzero:$imm),
(C_ADDI16SP X2, simm10_lsb0000nonzero:$imm)>;
+
+def : CompressPat<(QC_E_ADDI GPRNoX0:$rs1, GPRNoX0:$rs2, simm12:$imm),
+ (ADDI GPRNoX0:$rs1, GPRNoX0:$rs2, simm12:$imm)>;
+def : CompressPat<(QC_E_ANDI GPRNoX0:$rs1, GPRNoX0:$rs2, simm12:$imm),
+ (ANDI GPRNoX0:$rs1, GPRNoX0:$rs2, simm12:$imm)>;
+def : CompressPat<(QC_E_ORI GPRNoX0:$rs1, GPRNoX0:$rs2, simm12:$imm),
+ (ORI GPRNoX0:$rs1, GPRNoX0:$rs2, simm12:$imm)>;
+def : CompressPat<(QC_E_XORI GPRNoX0:$rs1, GPRNoX0:$rs2, simm12:$imm),
+ (XORI GPRNoX0:$rs1, GPRNoX0:$rs2, simm12:$imm)>;
+
+def : CompressPat<(QC_E_ADDAI GPRNoX0:$rd, simm12:$imm),
+ (ADDI GPRNoX0:$rd, GPRNoX0:$rd, simm12:$imm)>;
+def : CompressPat<(QC_E_ANDAI GPRNoX0:$rd, simm12:$imm),
+ (ANDI GPRNoX0:$rd, GPRNoX0:$rd, simm12:$imm)>;
+def : CompressPat<(QC_E_ORAI GPRNoX0:$rd, simm12:$imm),
+ (ORI GPRNoX0:$rd, GPRNoX0:$rd, simm12:$imm)>;
+def : CompressPat<(QC_E_XORAI GPRNoX0:$rd, simm12:$imm),
+ (XORI GPRNoX0:$rd, GPRNoX0:$rd, simm12:$imm)>;
} // let isCompressOnly = true, Predicates = [HasVendorXqcilia, IsRV32]
let Predicates = [HasVendorXqciac, IsRV32] in {
diff --git a/llvm/test/MC/RISCV/xqcilia-valid.s b/llvm/test/MC/RISCV/xqcilia-valid.s
index 872ffbe8126aa..2396271d7db69 100644
--- a/llvm/test/MC/RISCV/xqcilia-valid.s
+++ b/llvm/test/MC/RISCV/xqcilia-valid.s
@@ -123,3 +123,43 @@ qc.e.addai x2, 48
# CHECK-NOALIAS: c.andi s1, -1
# CHECK-ENC: encoding: [0xfd,0x98]
qc.e.andai x9, 4294967295
+
+# CHECK-ALIAS: addi t0, t2, 400
+# CHECK-NOALIAS: addi t0, t2, 400
+# CHECK-ENC: encoding: [0x93,0x82,0x03,0x19]
+qc.e.addi x5, x7, 400
+
+# CHECK-ALIAS: andi t0, t2, 750
+# CHECK-NOALIAS: andi t0, t2, 750
+# CHECK-ENC: encoding: [0x93,0xf2,0xe3,0x2e]
+qc.e.andi x5, x7, 750
+
+# CHECK-ALIAS: ori t0, t2, 854
+# CHECK-NOALIAS: ori t0, t2, 854
+# CHECK-ENC: encoding: [0x93,0xe2,0x63,0x35]
+qc.e.ori x5, x7, 854
+
+# CHECK-ALIAS: xori t0, t2, -200
+# CHECK-NOALIAS: xori t0, t2, -200
+# CHECK-ENC: encoding: [0x93,0xc2,0x83,0xf3]
+qc.e.xori x5, x7, -200
+
+# CHECK-ALIAS: addi t2, t2, 400
+# CHECK-NOALIAS: addi t2, t2, 400
+# CHECK-ENC: encoding: [0x93,0x83,0x03,0x19]
+qc.e.addai x7, 400
+
+# CHECK-ALIAS: andi t2, t2, 750
+# CHECK-NOALIAS: andi t2, t2, 750
+# CHECK-ENC: encoding: [0x93,0xf3,0xe3,0x2e]
+qc.e.andai x7, 750
+
+# CHECK-ALIAS: ori t2, t2, 854
+# CHECK-NOALIAS: ori t2, t2, 854
+# CHECK-ENC: encoding: [0x93,0xe3,0x63,0x35]
+qc.e.orai x7, 854
+
+# CHECK-ALIAS: xori t2, t2, -200
+# CHECK-NOALIAS: xori t2, t2, -200
+# CHECK-ENC: encoding: [0x93,0xc3,0x83,0xf3]
+qc.e.xorai x7, -200
diff --git a/llvm/utils/TableGen/CompressInstEmitter.cpp b/llvm/utils/TableGen/CompressInstEmitter.cpp
index d8626a0ce9e43..afc892b068582 100644
--- a/llvm/utils/TableGen/CompressInstEmitter.cpp
+++ b/llvm/utils/TableGen/CompressInstEmitter.cpp
@@ -171,10 +171,6 @@ bool CompressInstEmitter::validateTypes(const Record *DagOpType,
bool IsSourceInst) {
if (DagOpType == InstOpType)
return true;
- // Only source instruction operands are allowed to not match Input Dag
- // operands.
- if (!IsSourceInst)
- return false;
if (DagOpType->isSubClassOf("RegisterClass") &&
InstOpType->isSubClassOf("RegisterClass")) {
@@ -258,9 +254,9 @@ void CompressInstEmitter::addDagOperandMapping(const Record *Rec,
continue;
}
// Validate that Dag operand type matches the type defined in the
- // corresponding instruction. Operands in the input Dag pattern are
- // allowed to be a subclass of the type specified in corresponding
- // instruction operand instead of being an exact match.
+ // corresponding instruction. Operands in the input and output Dag
+ // patterns are allowed to be a subclass of the type specified in the
+ // corresponding instruction operand instead of being an exact match.
if (!validateTypes(DI->getDef(), OpndRec, IsSourceInst))
PrintFatalError(Rec->getLoc(),
"Error in Dag '" + Dag->getAsString() +
|
|
@llvm/pr-subscribers-mc Author: Sudharsan Veeravalli (svs-quic) ChangesWe have some 48-bit instructions in the For eg. the I think we can remove the check that only source instruction operands can mismatch with the corresponding DAG operands and rely on the fact that we check if the DAG register operand type is a subclass of the instruction register operand type. Full diff: https://github.com/llvm/llvm-project/pull/148660.diff 3 Files Affected:
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoXqci.td b/llvm/lib/Target/RISCV/RISCVInstrInfoXqci.td
index 0f222988114b8..4cbb3085207f9 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfoXqci.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoXqci.td
@@ -1638,6 +1638,24 @@ def : CompressPat<(QC_E_ADDAI X2, simm10_lsb0000nonzero:$imm),
(C_ADDI16SP X2, simm10_lsb0000nonzero:$imm)>;
def : CompressPat<(QC_E_ADDI X2, X2, simm10_lsb0000nonzero:$imm),
(C_ADDI16SP X2, simm10_lsb0000nonzero:$imm)>;
+
+def : CompressPat<(QC_E_ADDI GPRNoX0:$rs1, GPRNoX0:$rs2, simm12:$imm),
+ (ADDI GPRNoX0:$rs1, GPRNoX0:$rs2, simm12:$imm)>;
+def : CompressPat<(QC_E_ANDI GPRNoX0:$rs1, GPRNoX0:$rs2, simm12:$imm),
+ (ANDI GPRNoX0:$rs1, GPRNoX0:$rs2, simm12:$imm)>;
+def : CompressPat<(QC_E_ORI GPRNoX0:$rs1, GPRNoX0:$rs2, simm12:$imm),
+ (ORI GPRNoX0:$rs1, GPRNoX0:$rs2, simm12:$imm)>;
+def : CompressPat<(QC_E_XORI GPRNoX0:$rs1, GPRNoX0:$rs2, simm12:$imm),
+ (XORI GPRNoX0:$rs1, GPRNoX0:$rs2, simm12:$imm)>;
+
+def : CompressPat<(QC_E_ADDAI GPRNoX0:$rd, simm12:$imm),
+ (ADDI GPRNoX0:$rd, GPRNoX0:$rd, simm12:$imm)>;
+def : CompressPat<(QC_E_ANDAI GPRNoX0:$rd, simm12:$imm),
+ (ANDI GPRNoX0:$rd, GPRNoX0:$rd, simm12:$imm)>;
+def : CompressPat<(QC_E_ORAI GPRNoX0:$rd, simm12:$imm),
+ (ORI GPRNoX0:$rd, GPRNoX0:$rd, simm12:$imm)>;
+def : CompressPat<(QC_E_XORAI GPRNoX0:$rd, simm12:$imm),
+ (XORI GPRNoX0:$rd, GPRNoX0:$rd, simm12:$imm)>;
} // let isCompressOnly = true, Predicates = [HasVendorXqcilia, IsRV32]
let Predicates = [HasVendorXqciac, IsRV32] in {
diff --git a/llvm/test/MC/RISCV/xqcilia-valid.s b/llvm/test/MC/RISCV/xqcilia-valid.s
index 872ffbe8126aa..2396271d7db69 100644
--- a/llvm/test/MC/RISCV/xqcilia-valid.s
+++ b/llvm/test/MC/RISCV/xqcilia-valid.s
@@ -123,3 +123,43 @@ qc.e.addai x2, 48
# CHECK-NOALIAS: c.andi s1, -1
# CHECK-ENC: encoding: [0xfd,0x98]
qc.e.andai x9, 4294967295
+
+# CHECK-ALIAS: addi t0, t2, 400
+# CHECK-NOALIAS: addi t0, t2, 400
+# CHECK-ENC: encoding: [0x93,0x82,0x03,0x19]
+qc.e.addi x5, x7, 400
+
+# CHECK-ALIAS: andi t0, t2, 750
+# CHECK-NOALIAS: andi t0, t2, 750
+# CHECK-ENC: encoding: [0x93,0xf2,0xe3,0x2e]
+qc.e.andi x5, x7, 750
+
+# CHECK-ALIAS: ori t0, t2, 854
+# CHECK-NOALIAS: ori t0, t2, 854
+# CHECK-ENC: encoding: [0x93,0xe2,0x63,0x35]
+qc.e.ori x5, x7, 854
+
+# CHECK-ALIAS: xori t0, t2, -200
+# CHECK-NOALIAS: xori t0, t2, -200
+# CHECK-ENC: encoding: [0x93,0xc2,0x83,0xf3]
+qc.e.xori x5, x7, -200
+
+# CHECK-ALIAS: addi t2, t2, 400
+# CHECK-NOALIAS: addi t2, t2, 400
+# CHECK-ENC: encoding: [0x93,0x83,0x03,0x19]
+qc.e.addai x7, 400
+
+# CHECK-ALIAS: andi t2, t2, 750
+# CHECK-NOALIAS: andi t2, t2, 750
+# CHECK-ENC: encoding: [0x93,0xf3,0xe3,0x2e]
+qc.e.andai x7, 750
+
+# CHECK-ALIAS: ori t2, t2, 854
+# CHECK-NOALIAS: ori t2, t2, 854
+# CHECK-ENC: encoding: [0x93,0xe3,0x63,0x35]
+qc.e.orai x7, 854
+
+# CHECK-ALIAS: xori t2, t2, -200
+# CHECK-NOALIAS: xori t2, t2, -200
+# CHECK-ENC: encoding: [0x93,0xc3,0x83,0xf3]
+qc.e.xorai x7, -200
diff --git a/llvm/utils/TableGen/CompressInstEmitter.cpp b/llvm/utils/TableGen/CompressInstEmitter.cpp
index d8626a0ce9e43..afc892b068582 100644
--- a/llvm/utils/TableGen/CompressInstEmitter.cpp
+++ b/llvm/utils/TableGen/CompressInstEmitter.cpp
@@ -171,10 +171,6 @@ bool CompressInstEmitter::validateTypes(const Record *DagOpType,
bool IsSourceInst) {
if (DagOpType == InstOpType)
return true;
- // Only source instruction operands are allowed to not match Input Dag
- // operands.
- if (!IsSourceInst)
- return false;
if (DagOpType->isSubClassOf("RegisterClass") &&
InstOpType->isSubClassOf("RegisterClass")) {
@@ -258,9 +254,9 @@ void CompressInstEmitter::addDagOperandMapping(const Record *Rec,
continue;
}
// Validate that Dag operand type matches the type defined in the
- // corresponding instruction. Operands in the input Dag pattern are
- // allowed to be a subclass of the type specified in corresponding
- // instruction operand instead of being an exact match.
+ // corresponding instruction. Operands in the input and output Dag
+ // patterns are allowed to be a subclass of the type specified in the
+ // corresponding instruction operand instead of being an exact match.
if (!validateTypes(DI->getDef(), OpndRec, IsSourceInst))
PrintFatalError(Rec->getLoc(),
"Error in Dag '" + Dag->getAsString() +
|
lenary
reviewed
Jul 14, 2025
topperc
added a commit
to topperc/llvm-project
that referenced
this pull request
Jul 25, 2025
Instead of using the Record from the instruction definition, use the Record specified in the CompressPat DAG. This will allow us to use Records that are subsets of both the source and destination. I want to use this to merge the C_*_HINT instructions back into their regular non-HINT versions, but prevent those encodings from being part of compress/uncompress. For example, we will use GPRNoX0 for the C_ADDI CompressPat while both C_ADDI and ADDI will use GPR in their instruction definitions. To do this I've recorded the original DAG Record in the OperandMap using a struct in the union to represent the 3 fields needed for an Operand. Previously we stored TiedOpIdx outside the union, but only used it for Operand. There is a verification hole here where we don't have any way to check that an immediate predicate is a subset of an instruction predicate at tablegen time. Prior to llvm#148660 we had this hole in one direction, but that patch made it in two directions. I'm not sure if this patch makes it any worse. Now we're using what is in the CompressPat where before we were using whatever was in the instructions and ignoring the predicate in the CompressPat.
topperc
added a commit
that referenced
this pull request
Jul 25, 2025
…#150664) Instead of using the Record from the instruction definition, use the Record specified in the CompressPat DAG. This will allow us to use Records that are subsets of both the source and destination. I want to use this to merge the C_*_HINT instructions back into their regular non-HINT versions, but prevent those encodings from being part of compress/uncompress. For example, we will use GPRNoX0 for the C_ADDI CompressPat while both C_ADDI and ADDI will use GPR in their instruction definitions. To do this I've recorded the original DAG Record in the OperandMap using a struct in the union to represent the 3 fields needed for an Operand. Previously we stored TiedOpIdx outside the union, but only used it for Operand. There is a verification hole here where we don't have any way to check that an immediate predicate is a subset of an instruction predicate at tablegen time. Prior to #148660 we had this hole in one direction, but that patch made it in two directions. I'm not sure if this patch makes it any worse. Now we're using what is in the CompressPat where before we were using whatever was in the instructions and ignoring the predicate in the CompressPat.
mahesh-attarde
pushed a commit
to mahesh-attarde/llvm-project
that referenced
this pull request
Jul 28, 2025
…llvm#150664) Instead of using the Record from the instruction definition, use the Record specified in the CompressPat DAG. This will allow us to use Records that are subsets of both the source and destination. I want to use this to merge the C_*_HINT instructions back into their regular non-HINT versions, but prevent those encodings from being part of compress/uncompress. For example, we will use GPRNoX0 for the C_ADDI CompressPat while both C_ADDI and ADDI will use GPR in their instruction definitions. To do this I've recorded the original DAG Record in the OperandMap using a struct in the union to represent the 3 fields needed for an Operand. Previously we stored TiedOpIdx outside the union, but only used it for Operand. There is a verification hole here where we don't have any way to check that an immediate predicate is a subset of an instruction predicate at tablegen time. Prior to llvm#148660 we had this hole in one direction, but that patch made it in two directions. I'm not sure if this patch makes it any worse. Now we're using what is in the CompressPat where before we were using whatever was in the instructions and ignoring the predicate in the CompressPat.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We have some 48-bit instructions in the
Xqcispec that currently cannot be compressed to their 32-bit variants due to the constraint inCompressInstEmitteron destination instruction operands not being allowed to mismatch with the DAG operands.For eg. the
QC_E_ADDIinstruction can be compressed to theADDIinstruction when the immediate is signed-12 bit but this is currently not possible since theQC_E_ADDIinstruction hasGPRNoX0register operands while theADDIinstruction hasGPRregister operands leading to an operand type validation error.I think we can remove the check that only source instruction operands can mismatch with the corresponding DAG operands and rely on the fact that we check if the DAG register operand type is a subclass of the instruction register operand type.